任务3 bugzero by flymianmian · Pull Request #1 · EthanLin-TWer/bugszero-java

flymianmian · 2020-02-02T09:41:18Z

bug修复的时候有几步有点大。。

EthanLin-TWer · 2020-02-04T13:18:18Z

这个争取明天看完哈，看了一下最终结果好像跟我提供那分支差不多。

问一下学员想法，觉得提供一个参考分支是不是有必要呢？

EthanLin-TWer

为了达到原子提交的效果，这个提交最好和下个修改测试的提交合并一起提交。这样回退/回滚的时候测试仍然能在提交间保持运行通过。

前面做小调整修修的lint的节奏很好。

EthanLin-TWer · 2020-02-08T14:49:08Z

src/main/java/com/adaptionsoft/games/Game.java


 public class Game {
    ArrayList<String> players = new ArrayList<>();
+    ArrayList<Player> tempPlayers = new ArrayList<>();


赞！“旧的不变，新的创建”。

EthanLin-TWer · 2020-02-08T14:52:52Z

src/main/java/com/adaptionsoft/games/Player.java

 public class Player {
    private String name;
-    private int place = 0;
+    public int place = 0;


嗯，字段搬移过来的时候先设置为public是简化“替换使用点(getter)”的常见办法，可以使替换getter的时候步子变得更小。

EthanLin-TWer · 2020-02-08T15:01:53Z

src/main/java/com/adaptionsoft/games/Game.java


    private void gainGoldCoin() {
-        goldCoins[currentPlayer]++;
+        tempPlayers.get(currentPlayer).goldCoin++;


不知道这个地方做的时候会不会感觉步子大？

一般套路是在setter的地方同样“旧的不变，新的创建”一下，再把getter替换到新的字段上，最后删除老的setter：

goldCoins[currentPlayer]++; tempPlayers.get(currentPlayer).goldCoin++; // 先同时set到新旧两个地方

然后再做下面getter部分的替换，最后删除上面第一行的旧setter。

EthanLin-TWer · 2020-02-08T15:05:02Z

src/main/java/com/adaptionsoft/games/Game.java

 public class Game {
    ArrayList<String> players = new ArrayList<>();
    ArrayList<Player> tempPlayers = new ArrayList<>();
-    int[] goldCoins = new int[6];


还没看后面的提交，不过这个地方完成替换以后一般习惯是先用getGoldCoin()把public字段先封装起来。这个提交 12a280a 也是一样的。

EthanLin-TWer · 2020-02-08T15:11:00Z

src/main/java/com/adaptionsoft/games/Player.java

+    public void gainGoldCoin() {
+        goldCoin++;
+        System.out.println(name + " now has " + goldCoin + " Gold Coins.");
+    }


这个地方可不必手搬，见下个commit comment。

EthanLin-TWer · 2020-02-08T15:28:10Z

src/main/java/com/adaptionsoft/games/Game.java

-                + tempPlayers.get(currentPlayer).goldCoin
-                + " Gold Coins.");
-    }
-


这三个提交做的事是把gainGoldCoin()的行为搬移到Player上，在搬移上有自动化的手法/快捷键可以提高效率，也是一种常见的处理思路：

先利用“提炼变量”(《重构2》6.3节, cmd + option + v/ctrl + alt + v)提炼出currentPlayer来：

private void gainGoldCoin() { final Player player = tempPlayers.get(currentPlayer); player.goldCoin++; System.out.println(getCurrentPlayer() + " now has " + player.goldCoin + " Gold Coins."); }

将剩余部分的方法体利用“提炼方法”（《重构2》6.1节，cmd + option + m/ctrl + alt + m）提炼出去：

private void gainGoldCoin() { final Player player = tempPlayers.get(currentPlayer); gainGoldCoin(player); } private void gainGoldCoin(Player player) { player.goldCoin++; System.out.println(getCurrentPlayer() + " now has " + player.goldCoin + " Gold Coins."); }

对getCurrentPlayer()方法应用“内联函数”(《重构2》6.2节，cmd + option + n/control + alt + n)，因为getCurrentPlayer()对Player为private不可见：

private void gainGoldCoin(Player player) { player.goldCoin++; System.out.println(player.getName() + " now has " + player.goldCoin + " Gold Coins."); }

至此gainGoldCoin()方法中的数据都来源于Player，对该方法应用“搬移函数”(《重构2》8.1节，F6)，将方法搬移到Player类上：

private void gainGoldCoin() { final Player player = tempPlayers.get(currentPlayer); player.gainGoldCoin(); }

完成搬移，对方法做些后处理：先inline player，再inline整个方法：

private void gainGoldCoin() { tempPlayers.get(currentPlayer).gainGoldCoin(); }

public boolean wasCorrectlyAnswered() { if (tempPlayers.get(currentPlayer).isInPenaltyBox) { if (isGettingOutOfPenaltyBox) { ... nextPlayer(); tempPlayers.get(currentPlayer).gainGoldCoin(); return didPlayerWin(); } else { ... } } else { ... } }

这里有两个点：

“先提炼，后内联”：提炼出可供自动化搬移的结构，搬移后再内联回来。同样也是对过程的设计

过程需要处理下不在目标函数作用域中的变量/函数，在上面就是getCurrentPlayer()

手法虽然看起来很长，但其实练熟了速度更快，还不容易出错。

EthanLin-TWer · 2020-02-08T23:06:21Z

src/main/java/com/adaptionsoft/games/Player.java

+
+    public boolean isWin() {
+        return goldCoin >= 6;
+    }


isWin()可跟上述手法同理搬运。然后搞到这就发现两次为了搬运做了提炼getCurrentPlayer()方法的操作，不如重构完就提炼出这个方法。我做完这个提炼后发现一共有10处地方在用getCurrentPlayer()，也能一定程度为后面“搬移哪些字段”做提示。

后面提交还有些也同理，都可以用这个方法进行搬运，如penaltyBox几个处理方法那块。

EthanLin-TWer · 2020-02-08T23:41:57Z

src/main/java/com/adaptionsoft/games/Game.java

+        System.out.println("The category is " + currentCategory.getValue());
+        System.out.println(questionMap.get(currentCategory).removeFirst());
+    }
+


这里相当于直接抄了份新方法，改完以后直接把调用点切换过去，而没有在新旧方法中间建立桥梁。那如果这次调用点切换完测试挂了，你能怎么办？回滚copy的所有新东西？调试？因没有搭建桥梁故，所以这种手法不够小步。

我可能会这么做：

先对原来的currentCategory()应用提炼函数（6.1, cmd + option + m/control + alt + m）：

private String currentCategory() { return temp_currentCategory(); } private String temp_currentCategory() { if (getCurrentPlace() == 0) return "Pop"; if (getCurrentPlace() == 4) return "Pop"; if (getCurrentPlace() == 8) return "Pop"; if (getCurrentPlace() == 1) return "Science"; if (getCurrentPlace() == 5) return "Science"; if (getCurrentPlace() == 9) return "Science"; if (getCurrentPlace() == 2) return "Sports"; if (getCurrentPlace() == 6) return "Sports"; if (getCurrentPlace() == 10) return "Sports"; return "Rock"; }

修改temp_currentCategory()的返回值类型为Category，在旧方法中间将其转换为String并保持功能不变：

private String currentCategory() { return temp_currentCategory().getValue(); } private Category temp_currentCategory() { if (getCurrentPlace() == 0) return Category.POP; if (getCurrentPlace() == 4) return Category.POP; if (getCurrentPlace() == 8) return Category.POP; if (getCurrentPlace() == 1) return Category.SCIENCE; if (getCurrentPlace() == 5) return Category.SCIENCE; if (getCurrentPlace() == 9) return Category.SCIENCE; if (getCurrentPlace() == 2) return Category.SPORTS; if (getCurrentPlace() == 6) return Category.SPORTS; if (getCurrentPlace() == 10) return Category.SPORTS; return Category.ROCK; }

对调用点也先做“旧的不变，新的创建”的操作：

private void askQuestion() { String currentCategory = currentCategory(); + Category temp = Category.valueOf(currentCategory.toUpperCase()); System.out.println("The category is " + currentCategory); ... }

先替换一个调用点试试水：

private void askQuestion() { String currentCategory = currentCategory(); Category temp = Category.valueOf(currentCategory.toUpperCase()); - System.out.println("The category is " + currentCategory); + System.out.println("The category is " + temp.getValue()); ... }

4测试通过的话再替换其他引用点，不通过的话出错范围只有1行，回滚之找找原因：

private void askQuestion() { - String currentCategory = currentCategory(); Category temp = Category.valueOf(currentCategory.toUpperCase()); System.out.println("The category is " + temp.getValue()); if (temp == Category.POP) System.out.println(questionMap.get(Category.POP).removeFirst()); if (temp == Category.SCIENCE) System.out.println(questionMap.get(Category.SCIENCE).removeFirst()); if (temp == Category.SPORTS) System.out.println(questionMap.get(Category.SPORTS).removeFirst()); if (temp == Category.ROCK) System.out.println(questionMap.get(Category.ROCK).removeFirst()); }

清理，先内联String currentCategory变量，最后删除无用的currentCategory()并将temp_currentCategory()重命名回来：

private void askQuestion() { Category temp = temp_currentCategory(); System.out.println("The category is " + temp.getValue()); if (temp == Category.POP) System.out.println(questionMap.get(Category.POP).removeFirst()); if (temp == Category.SCIENCE) System.out.println(questionMap.get(Category.SCIENCE).removeFirst()); if (temp == Category.SPORTS) System.out.println(questionMap.get(Category.SPORTS).removeFirst()); if (temp == Category.ROCK) System.out.println(questionMap.get(Category.ROCK).removeFirst()); }

这里的重点是要在新旧函数中间搭建调用桥梁，这样后续才能每改一两行代码都跑测试验证。copy整份方法及调用点并直接替换的方式，在方法更长、更复杂、调用点更多的场景下就会步子很大，增加出错概率和排查范围。

EthanLin-TWer

这个提交是不是也能用之前搬移方法的方式做？

EthanLin-TWer · 2020-02-09T15:12:28Z

src/main/java/com/adaptionsoft/games/Category.java

+    SPORTS("Sports"),
+    ROCK("Rock"),
+    BLUES("Blues"),
+    HISTORY("History");


不错哟，做到了只需要加一行代码就能新增特性。

EthanLin-TWer · 2020-02-09T15:13:36Z

src/main/java/com/adaptionsoft/games/Category.java

+    public Category getCurrentCategory(int place) {
+        int index = place % Category.values().length;
+        return Category.values()[index];
+    }


同理直接用F6快捷键搬过来就行，不需要手搬。

EthanLin-TWer · 2020-02-09T15:14:50Z

src/main/java/com/adaptionsoft/games/Game.java


    private void askQuestion() {
-        Category currentCategory = currentCategory();
+        Category currentCategory = Category.getCurrentCategory(getCurrentPlace());


方法名是否可尝试简短些？例如：

Category currentCategory = Category.in(getCurrentPlace());

EthanLin-TWer

这三个提交 4c6f0d8 912615a 39ead3d 步子大了，真正执行替换时 e7d61bc 应该做不到每改一行代码都能运行测试吧？

替换方式可以像前面讲的，在setter的地方多set一次到PlayerContainer中去，再替换getter，最后删除一个字段，完成替换。然后再进行其他字段的替换。一次只搬移一个字段。

可参考《重构2》8.2节搬移字段（Move Field）。

EthanLin-TWer · 2020-02-09T15:25:02Z

src/main/java/com/adaptionsoft/games/Player.java

-    public int place = 0;
-    public int goldCoin = 0;
+    private int place = 0;
+    private int goldCoin = 0;


嗯，这个地方把字段用getter函数封装起来了很好。

EthanLin-TWer · 2020-02-09T15:29:46Z

Review done。

一些地方的手法还有可打磨的地方，不过总体的步子和手法应用还是很不错的。在执行“旧的不变，新的创建”时，需要更多依赖快捷键和IDE帮你做提炼并搭建好中间的调用桥梁，不需要手动去做创建，这样有利于进一步缩小步子，使重构更可控。

继续精益求精。很棒哟~

来一 added 30 commits February 2, 2020 12:41

重构：修改不规范范型容器声明

479b7ae

重构：重命名 purse->goldCoins

96eff23

重构：将字符串比较 == -> equals

b9c45d2

重构：去掉多余局部变量winner

d9bc927

重构：修改拼写错误 corrent -> correct

eb53f63

重构：修改测试中的拼写错误 corrent -> correct

6324eb2

重构：拆分movePlayerAndAskQuestion方法为 movePlayer和askQuestion两个方法，一个方法只做一件事

20c90ce

重构：提取 nextPlayer 方法

5dee0a0

重构：提取 gainGoldCoin 方法

1d6c45e

重构：提取 getOutOfPenaltyBox 方法

bbef036

重构：提取 stayInPenaltyBox 方法

508dbcb

重构：提取 getCurrentPlayer 方法

10a71ba

重构：创建 Player 类用于封装玩家信息

f863303

重构：创建 tempPlayers 容器

61c8615

重构：将 name 属性移入Player类，并在addPlayer时初始化

3a5b527

重构：替换 getCurrentPlayer 中 players 为 tempPlayers

7117405

重构：为 Player 类添加place字段

fd17f34

重构：提取 getCurrentPlace 方法

92ca351

重构：将 places 替换为 tempPlayers.get().place

b3320b5

重构：去掉 places

12a280a

重构：为 Player 增加 goldCoin 字段

0001bfc

重构：将 goldCoins 替换为 tempPlayers.get().goldCoin

bc3c833

重构：去掉 goldCoin

0c4b59d

重构：为 Player 增加 isInPenaltyBox 字段

2d6fb49

重构：将 isInPenaltyBox 替换为 tempPlayers.get().isInPenaltyBox

4df4924

重构：去掉isInPenaltyBox

78acc7a

重构：去掉howManyPlayers

a90554f

重构：为 Player 添加 moveTo 方法

0b78454

重构：将 movePlayer 方法替换为 tempPlayers.get(currentPlayer).moveTo

3e78b6e

重构：去掉 movePlayer 方法

d8fe767

来一 added 12 commits February 2, 2020 16:54

重构：在 PlayerContainer 类中创建 nextPlayer 方法

912615a

重构：在 PlayerContainer 类中创建 getCurrentPlayer 方法

39ead3d

重构：替换 ArrayList<Player> 为 PlayerContainer

e7d61bc

重构：将Player中place字段改为private并添加getter

945283a

重构：将Player中isInPenaltyBox字段改为private并添加getter

c794fc3

重构：将 stayInPenaltyBox 移入player类中

2773b68

重构：创建 QuestionContainer 类，封装question HashMap

3b260cb

重构：在 QuestionContainer 类中创建 getNextQuestion 方法

e100ed8

重构：在 QuestionContainer 类中创建构造器

924b0a8

重构：替换Game 类中 questionMap 为 QuestionContainer

a7e41e1

重构：拆分 roll 方法中的条件判断语句，解除if之间的嵌套

afbd2ad

重构：去掉只有一行的getXXX方法，将Game的方法减少为6个

5163db4

flymianmian requested a review from EthanLin-TWer February 2, 2020 09:41

EthanLin-TWer reviewed Feb 8, 2020

View reviewed changes

EthanLin-TWer reviewed Feb 9, 2020

View reviewed changes

Conversation

flymianmian commented Feb 2, 2020

Uh oh!

EthanLin-TWer commented Feb 4, 2020

Uh oh!

EthanLin-TWer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanLin-TWer Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanLin-TWer Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanLin-TWer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanLin-TWer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanLin-TWer commented Feb 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EthanLin-TWer Feb 8, 2020 •

edited

Loading

EthanLin-TWer Feb 8, 2020 •

edited

Loading

EthanLin-TWer left a comment •

edited

Loading